Conversation
…erminal failure, photo limits
- main.tsx: assign window.deferredInstallPrompt inside the handler so consumers
see the captured event instead of a frozen null.
- vite-env.d.ts: type BeforeInstallPromptEvent and Window.deferredInstallPrompt.
- lib/userSettings.ts: single shim for alert-sounds / auto-location / offline-mode
toggles (was scattered localStorage reads with inconsistent defaults).
- useFcmToken: play /notification.wav on incoming FCM message when alert sounds
are enabled — previously the toggle did nothing.
- Step2WhoWhere: respect getAutoLocationEnabled() — previously hard-coded true.
- SettingsPage: replace misleading "We will email your data" copy with an honest
"Coming soon" badge until Cluster 6 lands the real backend.
- RevealSheet: add failed_terminal variant with red banner, hotline-first copy
("We could not send. Please call now."), and direct tel: hand-off.
SubmitReportForm now routes terminal state to that variant instead of lying
with the retryable copy. Replace fake 2:14 PM timeline literals with the
current time.
- Step1Evidence: reject photos > 5 MB or non-image MIME (accept attribute is a
hint only). Surfaces error inline + 3 unit tests.
- pages/__tests__/LoginPage: fix 2 failing tests by using function keyword in
the RecaptchaVerifier mock so Vitest 4 can call it as a constructor.
- useResumeRegistration + RegisterPage: detect phone-linked auth user without
a citizen doc on bootstrap and resume registration at the consent step
instead of stranding the user.
All 316 tests pass; lint + typecheck green.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…tion, FCM rollback
- useSubmissionMachine: add 2s/4s/8s/...30s exponential backoff between
retries (capped at 30s). Previous instant retry burned all 3 attempts in
under a second on flaky networks. Backoff function exported for tests.
- backoff.test.ts: 3 unit tests covering base, growth, and cap.
- LookupScreen: stop surfacing raw error.message to users. Map known callable
error codes (not-found / permission-denied / unauthenticated /
resource-exhausted) to friendly strings; default falls back to a generic
message that suggests calling the hotline. permission-denied is mapped to
the same string as not-found to avoid leaking existence.
- useFcmToken:
* Replace getDoc + conditional updateDoc with setDoc({merge: true}) on both
requestPermission success and disable paths. Saves a round-trip and
removes a silent "doc missing" branch.
* On requestPermission failure, complete the rollback: deleteToken (already
there) + unsubscribeFromAlerts({ token }) + clear users/{uid}.fcmToken +
cleanup the onMessage listener if it was attached. Previously the topic
+ Firestore could be left out of sync.
* Track issuedTokenValue so the rollback unsubscribe call uses the actual
token instead of an empty string.
- useFcmToken.test: switch from updateDoc mock to setDoc mock; assert merge
option is set in both call sites.
All 319 tests pass; lint + typecheck green.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…, tests
- Fix SW IndexedDB name mismatch: bantayog_drafts → bantayog-drafts
to match localforage instance with architecture warning
- Expand SW cache cleanup to delete legacy bantayog-shell-* caches
- Add data_exports/{uid}/{file} owner-read rule to storage.rules
- Add storage.rules tests for data_exports (owner, cross-user, unauth)
- Add 3 new test files: imageCompress, useMunicipalityContact,
useResumeRegistration (10 tests total)
- Remove dead REPORT_INBOX_PATH constant from sw.js
- Update docs/progress.md and docs/learnings.md
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds service-worker Background Sync for queued reports (IndexedDB drafts), client-side image compression and photo validation, exponential backoff retry scheduling, municipality-specific contact wiring, settings shims, a resume-registration hook, a synchronous data-export callable with signed URL returns, related Firestore Storage rule, and multiple component/hook/test updates. ChangesOffline-First Submission & Sync Pipeline
Municipality Config & UI Variants
Photo Validation & Upload Flow
Settings, FCM & Resume Flow
Backend Data Export & Rules
PWA Install & Types
Documentation & Planning
Sequence Diagram(s)sequenceDiagram
participant User
participant App as Citizen PWA
participant SW as Service Worker (sw.js)
participant IDB as IndexedDB (bantayog-drafts)
participant FirestoreREST as Firestore REST / report_inbox
participant Functions as requestDataExport callable
User->>App: Submit report (photo + fields)
App->>App: compressImage(file) or keep file
alt Online
App->>FirestoreREST: POST report_inbox (documentId)
FirestoreREST-->>App: 200 OK (publicRef)
App->>User: Show success RevealSheet (municipality contact)
else Offline or network error
App->>IDB: store draft with syncState='local_only'/'syncing'
App->>SW: register Background Sync 'submit-report'
App->>User: Show queued RevealSheet
Note over SW,IDB: Later, when sync event fires...
SW->>IDB: read drafts (syncState local_only/syncing)
SW->>FirestoreREST: submitDraft(draft) via REST
FirestoreREST-->>SW: success
SW->>IDB: updateDraftSyncState(..., 'synced')
end
User->>App: Request "Download my data"
App->>Functions: requestDataExport()
Functions->>FirestoreREST: aggregate profile + reports + media
Functions->>CloudStorage: upload envelope JSON + sign URL
CloudStorage-->>Functions: signed downloadUrl
Functions-->>App: { downloadUrl, expiresAt, reportCount, mediaCount }
App->>User: initiate download of bantayog-export.json
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/citizen-pwa/src/services/callables.ts (1)
19-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve machine-readable error details.
apps/citizen-pwa/src/pages/SettingsPage.tsxLines 101-106 branches on the export failure reason, but this wrapper replaces the original exception with a plainErrorand keeps only its message. That turns the cooldown path into brittle string matching and can easily collapse back to the generic toast. Re-throw the original callable error, or copy its code onto the wrapped error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/services/callables.ts` around lines 19 - 23, The catch block in apps/citizen-pwa/src/services/callables.ts currently throws a fresh Error (`throw new Error("Data export request failed: ...", { cause: err })`) which discards machine-readable details used by SettingsPage.tsx to branch on failure; instead either re-throw the original callable error (throw err) or when wrapping, copy the original error's discriminators (e.g. err.code or err.name) onto the new Error and keep err as the cause so callers can inspect those fields—update the throw site to preserve err.code (or rethrow) and ensure the new error's cause remains the original error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/citizen-pwa/public/sw.js`:
- Around line 101-123: The submitDraft serializer is using fields that don't
exist on the queued Draft shape: replace references to draft.publicLocation with
draft.location and stop forcing reporterUid to an empty string; instead read the
actual reporter identifier provided on the Draft (e.g. draft.reporterId or the
correct reporter field from your Draft type) and only include reporterUid when
present. Update the submitDraft function to use draft.location for the payload's
location property and to read/omit the reporter UID using the real Draft field
name so the background-sync payload matches writeWithTimeout(...) in
useSubmissionMachine.
- Around line 131-137: The fetch URL is targeting a document path and the body
uses raw JSON; change the URL in the fetch call that builds the Firestore REST
request so it posts to the collection path (remove /${draft.id} so use
.../documents/report_inbox?documentId=${draft.id}) and transform inboxDoc into
Firestore's Document format before JSON.stringify: wrap the payload in a
top-level "fields" object and convert each field value into Firestore Value
objects (e.g., stringValue, integerValue, mapValue) so the fetch body is a valid
Firestore Document; update the fetch invocation that references projectId,
draft.id and inboxDoc accordingly.
- Around line 64-78: The service worker assumes the 'syncState' index exists but
if the DB was created earlier by localforage it won't trigger onupgradeneeded
and the index/store will be missing; update openDraftsDB to open with a bumped
version (e.g., indexedDB.open(DB_NAME, 2)) and in the onupgradeneeded handler
check if the object store exists via
req.result.objectStoreNames.contains(DB_STORE) and create the DB_STORE if
missing, then create the 'syncState' index only if
!store.indexNames.contains('syncState'); also harden submitQueuedDrafts (and any
callers) to check that the object store and index exist before calling
store.index('syncState') and gracefully handle the missing index (e.g., skip
sync or return early). Ensure references to DB_NAME, DB_STORE, openDraftsDB,
submitQueuedDrafts, and the 'syncState' index are updated accordingly.
In `@apps/citizen-pwa/src/components/RevealSheet.tsx`:
- Around line 214-216: The SMS fallback currently interpolates
contact.smsShortCode raw in handleSmsFallback; sanitize/normalize the shortcode
the same way the hotline normalization does (e.g., strip all non-digits except
an optional leading '+', or call the existing helper used for hotline numbers)
before composing the `sms:` URI, and if the sanitized value is empty/invalid,
bail out and surface an error (e.g., processLogger/console.warn or show a toast)
instead of navigating. Ensure you reference handleSmsFallback and
contact.smsShortCode when making this change so the fallback uses the normalized
recipient.
In `@apps/citizen-pwa/src/components/SubmitReportForm/index.tsx`:
- Around line 80-82: When creating the photo variable in SubmitReportForm, don't
let compressImage(...) rejections abort submission; wrap the await
compressImage(formData.step1.photoFile) call in a try/catch and on any error set
photo to the original formData.step1.photoFile (or undefined if none) and log
the error; this preserves the existing client-side validation while falling back
safely when compressImage fails so draft creation continues.
In `@apps/citizen-pwa/src/components/SubmitReportForm/Step1Evidence.test.tsx`:
- Around line 43-50: The test currently only checks absence of an alert, which
doesn't prove the file was accepted; after calling renderStep1() and
fireEvent.change(input, { target: { files: [goodFile] } }) assert a positive UI
change that indicates setPhotoFile(file) ran — e.g. expect the uploaded filename
to be rendered (getByText('photo.jpg')) or a preview image is present
(getByAltText(/preview/i)), or the "Next"/submit button becomes enabled
(getByRole('button', { name: /next/i }).toBeEnabled()). Update
Step1Evidence.test.tsx to assert one of these success signals after using
makeFile and firing the change.
In `@apps/citizen-pwa/src/hooks/__tests__/backoff.test.ts`:
- Around line 4-19: Add an end-to-end hook test that exercises the useEffect in
useSubmissionMachine (not just backoffDelay) using fake timers: mount/use
renderHook on useSubmissionMachine (or a minimal component that calls it),
configure the submission to fail so the retry path runs, advance timers with
jest.useFakeTimers()/advanceTimersByTime and assert the retries are scheduled
with delays matching backoffDelay (2_000 then 4_000 then 8_000 ... capped at
30_000); this verifies the setTimeout scheduling inside the useEffect and
ensures the timing regression is covered.
In `@apps/citizen-pwa/src/hooks/useFcmToken.ts`:
- Around line 106-110: The Firestore updates set fcmToken but when
revoking/rolling back they only clear fcmToken, leaving stale fcmTokenUpdatedAt;
update every write that clears or nulls the token to also clear
fcmTokenUpdatedAt so both fields are set together; locate the calls to
setDoc/doc(db(), 'users', user.uid) (e.g., in useFcmToken's success path and the
rollback/disable paths) and ensure the payload sets { fcmToken: null,
fcmTokenUpdatedAt: null } (or omits both) consistently whenever the token is
removed.
In `@apps/citizen-pwa/src/hooks/useMunicipalityContact.ts`:
- Around line 28-37: The pickString helper currently treats strings of only
whitespace as valid; update pickString to trim the input before checking length
so whitespace-only values return undefined and fall back in toContact;
specifically modify pickString (used by toContact) to accept unknown, coerce to
string when typeof value === 'string', call value.trim(), and only return the
original (or trimmed) string if trimmed.length > 0, otherwise return undefined
so toContact's label, hotline, and smsShortCode fall back to their defaults.
In `@apps/citizen-pwa/src/hooks/useResumeRegistration.ts`:
- Around line 25-41: The current code uses only "active" to avoid acting after
unmount, but a stale getDoc() can still complete after auth changes and redirect
the new session; introduce a "latestUid" (or similar) variable outside the
onAuthStateChanged handler, set it to user.uid immediately when the callback
runs, then before calling getDoc capture that uid in a local constant (e.g.,
currentUid) and after the promise resolves or in the catch verify currentUid ===
latestUid (and active) before calling navigate or logging; update references to
user.uid/currentUid in the then/catch branches so only the most-recent auth
session can trigger navigate(`/register?${RESUME_QUERY}`, { replace: true }) or
log errors.
In `@apps/citizen-pwa/src/hooks/useSubmissionMachine.ts`:
- Around line 223-232: The retry delay uses backoffDelay(retryCountRef.current)
but retryCountRef.current is already incremented before scheduling, making the
first retry use index 1; change the calculation to use a zero-based retry index
by calling backoffDelay(Math.max(0, retryCountRef.current - 1)) where delay is
computed (around the setTimeout block in useSubmissionMachine.ts) so the first
retry uses backoffDelay(0) and subsequent retries map correctly; keep doSubmit,
setTimeout, and retryCountRef.current usage unchanged except for this index
adjustment.
- Around line 171-186: Replace the current ServiceWorker registration logic in
useSubmissionMachine so it awaits navigator.serviceWorker.ready (not
self.registration) and only attempts reg.sync.register('submit-report') when the
submission outcome/state from doSubmit() is one that should be retried by the
ServiceWorker (i.e., when the machine/state is 'queued' or 'failed_retryable'
and not when it's 'failed_terminal'); also guard for reg.sync availability and
swallow/register errors as a best-effort fallback. Ensure you update the code
paths around doSubmit()/submission state checks in useSubmissionMachine to
perform this conditional background-sync registration.
In `@apps/citizen-pwa/src/lib/__tests__/imageCompress.test.ts`:
- Around line 13-35: The tests currently only assert compressImage returns a
Promise or passes through small files, so update each spec to await
compressImage and assert the resolved value and behavior: for "returns a Blob
for files larger than 200KB" await the result from compressImage(largeFile) and
assert the resolved value is a Blob and not the original File (or that its size
is < original to indicate compression); for "accepts optional maxEdge and
quality parameters" await compressImage(file, { maxEdge: 800, quality: 0.5 })
and assert the result is a Blob and that size/metadata reflect the options
(e.g., different size than default); and for "defaults maxEdge to 1080 and
quality to 0.8" await compressImage(smallFile) and assert the resolved value
equals the input only when compression is a no-op otherwise verify expected Blob
properties. Use controlled image/canvas stubs or create deterministic image data
so canvas-based resizing runs reliably when invoking compressImage.
In `@apps/citizen-pwa/src/lib/imageCompress.ts`:
- Around line 13-17: The image compressor currently rejects on failures
(img.onerror, missing 2d context, or missing canvas.toBlob) causing a hard
failure instead of the documented graceful fallback; update img.onerror to call
URL.revokeObjectURL(img.src) and resolve with the original source File, and
change the failure branches around canvas.getContext('2d') and canvas.toBlob to
resolve with the original source File instead of reject; ensure every early-exit
path (onerror, missing context, missing toBlob, and any catch) revokes the
created object URL and returns the original file so the caller can proceed with
the uncompressed image.
In `@apps/citizen-pwa/src/pages/SettingsPage.tsx`:
- Around line 92-98: The current flow uses the cross-origin signed downloadUrl
returned by requestDataExport and sets a.download on an anchor (downloadUrl /
created <a>), which browsers ignore for cross-origin URLs; change the client to
fetch the signed downloadUrl, convert the response to a Blob, create an object
URL via URL.createObjectURL(blob), set the created anchor's href to that object
URL and its download to "bantayog-export.json", click the anchor and then revoke
the object URL, or alternatively modify the server-side export generator to
include Content-Disposition: attachment when creating the signed URL so the
browser will download the file; update the code paths that reference
requestDataExport, downloadUrl and the created anchor element accordingly.
In `@apps/citizen-pwa/src/services/callables.ts`:
- Around line 12-18: The current return in the function that awaits callable()
blindly casts result.data to a specific shape (downloadUrl, expiresAt,
reportCount, mediaCount); validate result.data before returning by checking that
those keys exist and have the expected types (string for downloadUrl, number for
expiresAt/reportCount/mediaCount) and throw or return a clear error if
validation fails; follow the same validation pattern used by registerCitizen()
in this file and reference the awaited variable callable and the returned
payload shape when adding the checks so malformed backend responses are rejected
rather than blindly cast.
In `@docs/no-need-to-schedule-graceful-quasar.md`:
- Around line 75-78: The fenced code blocks containing the constants
MAX_PHOTO_BYTES and ALLOWED_MIME (and the other block at lines 276-297) need a
language tag to satisfy markdownlint MD040; update the opening triple-backtick
lines to specify a language (e.g., ```js or ```javascript) for the blocks that
show JavaScript/TypeScript code so the linter recognises the language and the
warning is resolved.
- Line 396: Update the stale test count string "313 tests pass" to the current
value "318 tests pass" in the document line that shows the command "pnpm lint &&
pnpm typecheck && npx vitest run"; also scan the document for any other
occurrences of "313 tests" or similar test-count mentions and replace them with
"318" so the checklist accurately reflects the branch verification summary.
In `@functions/src/callables/request-data-export.ts`:
- Around line 76-87: The current read-then-write rate limiting using
existingQuery is raceable; update requestDataExport to perform an atomic
check-and-reserve by using a Firestore transaction (or a deterministic per-user
limiter document) so two concurrent calls cannot both proceed: inside a
transaction, read the 'data_exports' (or a dedicated 'data_export_limiter' doc
keyed by actor.uid), check for any pending/ready exports within RATE_LIMIT_MS,
and if none, create the pending export tracker (status 'pending') before
continuing; ensure you reference existingQuery, RATE_LIMIT_MS, actor.uid and
throw the same HttpsError('resource-exhausted', ...) when the transaction finds
a recent export.
- Around line 210-214: The catch block in the wrapper around
requestDataExportImpl currently rethrows non-HttpsError failures with the
original error message, potentially leaking internal details; update the catch
to (1) log the original error server-side (e.g., using console.error or a
logger) including the full error object, (2) continue to rethrow existing
HttpsError instances unchanged, and (3) for all other errors throw a new
HttpsError('internal', 'Internal server error') (or another fixed generic
message) instead of using err.message; refer to requestDataExportImpl and the
existing catch that constructs new HttpsError to locate and modify the logic.
- Around line 121-126: The media lookup uses an unbounded reportIds array with a
single where('reportId', 'in', reportIds) call which will fail when reportIds >
30; modify the code in request-data-export.ts (the block building reportIds and
calling db.collection('report_media').where('reportId','in',...)) to split
reportIds into batches of max 30 (e.g., chunk the reportIds array), perform the
query for each batch (await each mediaSnap or run them in parallel),
collect/merge all resulting docs before mapping to ExportedMedia and appending
to mediaItems, and ensure the final mediaItems contains entries from all
batches.
In `@infra/firebase/storage.rules`:
- Around line 37-39: The current rule for match /data_exports/{uid}/{file} uses
request.auth != null && request.auth.uid == uid which bypasses the shared
accountStatus == 'active' gate; replace that check with the shared helper by
changing the allow read condition to use isAuthed() (e.g., allow read: if
isAuthed() && request.auth.uid == uid or just isAuthed() if that helper already
enforces uid match) so suspended accounts are blocked and the accountStatus ==
'active' check is applied.
In `@packages/shared-validators/src/municipalities.ts`:
- Around line 19-21: The mdrrmoHotline zod validator (mdrrmoHotline) rejects
seeded numbers like "(054) 721-1216" because the regex requires the first
character to be '+' or a digit; update the regex to also accept a leading
parenthesis so the pattern allows phone numbers starting with '(', '+', or a
digit (e.g., adjust the start-of-string character class for mdrrmoHotline in the
municipalityDocSchema file to include '(') while preserving the rest of the
length/character checks.
---
Outside diff comments:
In `@apps/citizen-pwa/src/services/callables.ts`:
- Around line 19-23: The catch block in
apps/citizen-pwa/src/services/callables.ts currently throws a fresh Error
(`throw new Error("Data export request failed: ...", { cause: err })`) which
discards machine-readable details used by SettingsPage.tsx to branch on failure;
instead either re-throw the original callable error (throw err) or when
wrapping, copy the original error's discriminators (e.g. err.code or err.name)
onto the new Error and keep err as the cause so callers can inspect those
fields—update the throw site to preserve err.code (or rethrow) and ensure the
new error's cause remains the original error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9f4c4d56-67c3-4efc-b238-9e618de5e3da
📒 Files selected for processing (39)
apps/citizen-pwa/public/sw.jsapps/citizen-pwa/src/components/LookupScreen.tsxapps/citizen-pwa/src/components/RevealSheet.lazy.tsxapps/citizen-pwa/src/components/RevealSheet.tsxapps/citizen-pwa/src/components/SubmitReportForm/Step1Evidence.test.tsxapps/citizen-pwa/src/components/SubmitReportForm/Step1Evidence.tsxapps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsxapps/citizen-pwa/src/components/SubmitReportForm/index.tsxapps/citizen-pwa/src/components/ui/StatusBanner.tsxapps/citizen-pwa/src/hooks/__tests__/backoff.test.tsapps/citizen-pwa/src/hooks/__tests__/useFcmToken.test.tsxapps/citizen-pwa/src/hooks/__tests__/useMunicipalityContact.test.tsapps/citizen-pwa/src/hooks/__tests__/useResumeRegistration.test.tsapps/citizen-pwa/src/hooks/useFcmToken.tsapps/citizen-pwa/src/hooks/useMunicipalityContact.tsapps/citizen-pwa/src/hooks/useResumeRegistration.tsapps/citizen-pwa/src/hooks/useSubmissionMachine.tsapps/citizen-pwa/src/lib/__tests__/draftManager.test.tsapps/citizen-pwa/src/lib/__tests__/imageCompress.test.tsapps/citizen-pwa/src/lib/draftManager.tsapps/citizen-pwa/src/lib/imageCompress.tsapps/citizen-pwa/src/lib/localforage.tsapps/citizen-pwa/src/lib/photoUpload.tsapps/citizen-pwa/src/lib/userSettings.tsapps/citizen-pwa/src/main.tsxapps/citizen-pwa/src/pages/RegisterPage.tsxapps/citizen-pwa/src/pages/SettingsPage.tsxapps/citizen-pwa/src/pages/__tests__/LoginPage.test.tsxapps/citizen-pwa/src/routes.tsxapps/citizen-pwa/src/services/callables.tsapps/citizen-pwa/src/styles/globals.cssapps/citizen-pwa/src/vite-env.d.tsdocs/learnings.mddocs/no-need-to-schedule-graceful-quasar.mddocs/progress.mdfunctions/src/__tests__/storage.rules.test.tsfunctions/src/callables/request-data-export.tsinfra/firebase/storage.rulespackages/shared-validators/src/municipalities.ts
💤 Files with no reviewable changes (4)
- apps/citizen-pwa/src/lib/photoUpload.ts
- apps/citizen-pwa/src/lib/draftManager.ts
- apps/citizen-pwa/src/lib/localforage.ts
- apps/citizen-pwa/src/lib/tests/draftManager.test.ts
Service Worker: - Bump IDB version to 2, add store/index existence checks - Fix draft serialization: use draft.location (not publicLocation), read reporterUid/reporterId - Convert payload to Firestore REST Document format (fields/value objects) - Fix URL to collection path with documentId query param - Move SW sync registration into doSubmit for queued states only Functions: - request-data-export: Atomic rate limit via Firestore transaction - Batch media queries at 30 reportIds per Firestore in clause - Generic error message for internal failures (logs original) Storage Rules: - Use isAuthed() helper for data_exports reads (enforces active status) Shared Validators: - Allow leading parenthesis in mdrrmoHotline regex Citizen PWA Hooks: - useSubmissionMachine: Fix retry delay off-by-one (use backoffDelay(retryCount-1)) - useResumeRegistration: Guard stale auth with latestUid tracking - useMunicipalityContact: Trim whitespace in pickString - useFcmToken: Clear fcmTokenUpdatedAt alongside fcmToken Components: - SettingsPage: Download via fetch+blob+objectURL for cross-origin - RevealSheet: Sanitize SMS shortcode before composing URI - SubmitReportForm: Wrap compressImage in try/catch, fallback to original - callables: Validate response data shape, preserve original errors Tests & Docs: - Update callables.test.ts and imageCompress.test.ts for new behavior - Add positive assertion in Step1Evidence.test.tsx - Update test count 313→318, add language tags to code blocks Verification: pnpm lint, pnpm typecheck, npx vitest run (318 tests pass)
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
apps/citizen-pwa/src/lib/__tests__/imageCompress.test.ts (1)
13-28: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winLarge-file tests still don’t validate compression behavior.
Line 13-28 only checks
Promiseshape, so these specs can pass even if the large-file path never produces the expected result. Please await and assert resolved output under controlledImage/canvasstubs.As per coding guidelines, "Test Discipline: ... Verify tests actually run the code under test."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/lib/__tests__/imageCompress.test.ts` around lines 13 - 28, The tests only assert that compressImage(...) returns a Promise; update both cases to stub the environment (Image and canvas drawImage/toBlob) so the compression path actually runs, await the Promise from compressImage(largeFile) and assert the resolved value is a Blob or File (or has a size and type), and specifically assert the compressed output size is smaller than the original and the MIME type remains image/jpeg; do the same for the variant call compressImage(file, { maxEdge: 800, quality: 0.5 }) to await and assert resolved output matches expected shape and constraints; use the test helpers/stubs appropriate for happy-dom so the image/canvas operations do not hang and the assertions validate real compression behavior.apps/citizen-pwa/src/hooks/useResumeRegistration.ts (1)
27-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset
latestUidbefore early returns to fully block stale completions.At Line 32,
latestUidis updated only for phone-linked users. If auth flips to logged-out/anonymous beforegetDoc()resolves, the oldlatestUidcan still pass Line 39/44 and trigger stale side effects for the previous session.Suggested minimal fix
const unsubscribe = onAuthStateChanged(auth(), (user) => { if (!active) return + latestUid = user?.uid ?? null if (!user || user.isAnonymous) return if (!user.phoneNumber) return - latestUid = user.uid const currentUid = user.uid @@ .catch((err: unknown) => { - if (currentUid !== latestUid) return + if (!active || currentUid !== latestUid) return console.warn('[useResumeRegistration] users/{uid} read failed:', err) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/hooks/useResumeRegistration.ts` around lines 27 - 45, When handling auth state in the onAuthStateChanged callback, ensure you clear the shared latestUid before any early return so stale UIDs can't pass later checks in the asynchronous getDoc()/then/catch handlers; update the callback in useResumeRegistration to set latestUid = undefined (or null/empty string as used elsewhere) immediately when returning for !user, user.isAnonymous, or missing phoneNumber, keeping the assignment latestUid = user.uid only for valid phone-linked users and leaving the getDoc()/navigate logic unchanged.apps/citizen-pwa/public/sw.js (1)
116-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
reporterUidsource still doesn’t match the queuedDraftshape.
apps/citizen-pwa/src/services/draft-store.ts(Line 1-36 in provided context) defines noreporterUid/reporterId, so Line 116 will usually serialize''. This can diverge from the in-app submission payload and weaken identity/audit consistency.Minimal safer pattern
- const inboxDoc = { - reporterUid: draft.reporterUid ?? draft.reporterId ?? '', + const reporterUid = + typeof draft.reporterUid === 'string' + ? draft.reporterUid + : typeof draft.reporterId === 'string' + ? draft.reporterId + : undefined + const inboxDoc = { + ...(reporterUid ? { reporterUid } : {}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/public/sw.js` at line 116, The service worker is serializing reporterUid incorrectly and usually falling back to '' because the Draft shape in apps/citizen-pwa/src/services/draft-store.ts doesn't define reporterUid/reporterId; update the mapping in sw.js to derive reporterUid from the actual Draft reporter object first (e.g., check draft.reporter?.uid), then fall back to draft.reporterUid and draft.reporterId, and finally an empty string; update the line that sets reporterUid so it reads the reporter.uid when available to keep queued payloads consistent with in-app submission payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/citizen-pwa/public/sw.js`:
- Around line 145-149: The toFirestoreValue function currently encodes every JS
number as integerValue which breaks fractional coordinates; update to detect
integers vs non-integers (e.g., use Number.isInteger(value)) and return
integerValue (as String(value) to match existing style) for integers and
doubleValue for non-integers (fractional numbers like lat/lng), leaving other
branches (stringValue, booleanValue, nullValue) unchanged; locate the function
named toFirestoreValue and change the number-handling branch accordingly.
In `@apps/citizen-pwa/src/components/RevealSheet.tsx`:
- Around line 208-212: The handleCallHotline handler currently always navigates
even when contact.hotline normalizes to an empty/invalid string; update
handleCallHotline to validate the sanitized recipient before changing
window.location.href: compute telDigits as you already do
(contact.hotline.replace(/[^\d+]/g, '')), then if telDigits is falsy or does not
contain at least one digit (or only a leading '+') log a warning (e.g.,
console.warn) including the original contact.hotline and return early without
setting window.location.href; only call window.location.href =
`tel:${telDigits}` when the sanitized value passes validation.
In `@apps/citizen-pwa/src/hooks/useFcmToken.ts`:
- Around line 11-16: The empty catch blocks in useFcmToken around the alert
sound (the try/catch that constructs Audio and the .catch on audio.play())
silently swallow errors; update them to capture the error (e) and emit a short,
contextual diagnostic (e.g., process/local logger or console.warn/console.debug)
including the error message and that it occurred in useFcmToken alert sound
playback (and optionally add simple rate-limiting or debounce to avoid log
spam). Ensure you reference the Audio construction try/catch and the
audio.play().catch(...) in useFcmToken, pass the caught error into the logger,
and keep the user-facing behavior unchanged.
In `@apps/citizen-pwa/src/hooks/useSubmissionMachine.ts`:
- Around line 142-144: In useSubmissionMachine.ts, the empty catch after the
Background Sync registration swallows errors; update the catch in the Background
Sync registration block (the try/catch around registering sync) to log the
caught error with context instead of ignoring it — e.g., capture the thrown
error and call the project logger or console.error with a descriptive message
including that the Background Sync registration failed and include the error
object and any relevant state (e.g., scope or registration info) to aid
debugging.
In `@apps/citizen-pwa/src/services/callables.test.ts`:
- Around line 18-37: Add a negative test that simulates a malformed response
from the callable and asserts requestDataExport rejects/throws; specifically,
create a new it(...) that sets mockCall = vi.fn().mockResolvedValue({ data: { /*
missing or invalid fields, e.g. downloadUrl: null or missing expiresAt */ } })
and mockHttpsCallable.mockReturnValue(mockCall), then call await
expect(requestDataExport()).rejects.toThrow() (or the specific error your
validator throws) and also assert mockHttpsCallable was called with
('mocked-functions', 'requestDataExport') and mockCall was invoked; place this
alongside the existing happy-path test for requestDataExport.
In `@functions/src/callables/request-data-export.ts`:
- Around line 166-168: The empty catch in the media URL signing block inside
request-data-export.ts swallows all errors; update the try/catch to catch the
error (e.g., catch (err)), log the error with context (item id/storagePath and
operation like "signing media URL") using the existing logger (or
console.error), and only suppress/omit the URL for expected "not found" errors
while rethrowing or surfacing unexpected errors so regressions are visible;
locate the try/catch that attempts to sign storage URLs in the
request-data-export handler and replace the silent catch with this conditional
logging/handling.
---
Duplicate comments:
In `@apps/citizen-pwa/public/sw.js`:
- Line 116: The service worker is serializing reporterUid incorrectly and
usually falling back to '' because the Draft shape in
apps/citizen-pwa/src/services/draft-store.ts doesn't define
reporterUid/reporterId; update the mapping in sw.js to derive reporterUid from
the actual Draft reporter object first (e.g., check draft.reporter?.uid), then
fall back to draft.reporterUid and draft.reporterId, and finally an empty
string; update the line that sets reporterUid so it reads the reporter.uid when
available to keep queued payloads consistent with in-app submission payloads.
In `@apps/citizen-pwa/src/hooks/useResumeRegistration.ts`:
- Around line 27-45: When handling auth state in the onAuthStateChanged
callback, ensure you clear the shared latestUid before any early return so stale
UIDs can't pass later checks in the asynchronous getDoc()/then/catch handlers;
update the callback in useResumeRegistration to set latestUid = undefined (or
null/empty string as used elsewhere) immediately when returning for !user,
user.isAnonymous, or missing phoneNumber, keeping the assignment latestUid =
user.uid only for valid phone-linked users and leaving the getDoc()/navigate
logic unchanged.
In `@apps/citizen-pwa/src/lib/__tests__/imageCompress.test.ts`:
- Around line 13-28: The tests only assert that compressImage(...) returns a
Promise; update both cases to stub the environment (Image and canvas
drawImage/toBlob) so the compression path actually runs, await the Promise from
compressImage(largeFile) and assert the resolved value is a Blob or File (or has
a size and type), and specifically assert the compressed output size is smaller
than the original and the MIME type remains image/jpeg; do the same for the
variant call compressImage(file, { maxEdge: 800, quality: 0.5 }) to await and
assert resolved output matches expected shape and constraints; use the test
helpers/stubs appropriate for happy-dom so the image/canvas operations do not
hang and the assertions validate real compression behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cc532213-8bc9-4de7-bd17-fc24f6f4a8b5
📒 Files selected for processing (17)
apps/citizen-pwa/public/sw.jsapps/citizen-pwa/src/components/RevealSheet.tsxapps/citizen-pwa/src/components/SubmitReportForm/Step1Evidence.test.tsxapps/citizen-pwa/src/components/SubmitReportForm/index.tsxapps/citizen-pwa/src/hooks/useFcmToken.tsapps/citizen-pwa/src/hooks/useMunicipalityContact.tsapps/citizen-pwa/src/hooks/useResumeRegistration.tsapps/citizen-pwa/src/hooks/useSubmissionMachine.tsapps/citizen-pwa/src/lib/__tests__/imageCompress.test.tsapps/citizen-pwa/src/lib/imageCompress.tsapps/citizen-pwa/src/pages/SettingsPage.tsxapps/citizen-pwa/src/services/callables.test.tsapps/citizen-pwa/src/services/callables.tsdocs/no-need-to-schedule-graceful-quasar.mdfunctions/src/callables/request-data-export.tsinfra/firebase/storage.rulespackages/shared-validators/src/municipalities.ts
…89 - sw.js: toFirestoreValue uses doubleValue for non-integers; safer reporterUid pattern checking draft.reporter?.uid first - RevealSheet.tsx: validate sanitized hotline contains at least one digit before dialing - useFcmToken.ts: add diagnostic logging to alert sound catch blocks - useSubmissionMachine.ts: log Background Sync registration failures - callables.test.ts: add negative test for malformed requestDataExport response - request-data-export.ts: log media URL signing errors with context - useResumeRegistration.ts: reset latestUid before early returns to prevent stale auth completions Verification: pnpm lint, pnpm typecheck, npx vitest run (319 tests pass)
Summary
Post-implementation review fixes for the Citizen PWA hardening sweep (Clusters 1-6).
Changes
Service Worker
DB_NAMEfrombantayog_drafts→bantayog-draftsto match the app's localforage instance. Added architecture warning comment about localforage/raw-IDB schema mismatch.bantayog_shell_*and legacybantayog-shell-*caches.REPORT_INBOX_PATHwas unused.Storage Rules
data_exports/{uid}/{file}rule: Owner-only read (request.auth.uid == uid) + write-deny. Defense-in-depth even though signed URLs bypass rules.New Tests (3 files, 10 tests)
imageCompress.test.ts— 4 tests (small file passthrough, large file Promise, opts acceptance, defaults)useMunicipalityContact.test.ts— 4 tests (undefined id, snapshot update, error fallback, partial doc fallback)useResumeRegistration.test.ts— 3 tests (anonymous no-op, no-phone no-op, resume navigation)Documentation
docs/progress.mdwith test count (318) and post-impl-review fixes sectiondocs/learnings.mdwith 4 new learnings about IDB, rules, lint void-expressionVerification
pnpm lint && pnpm typecheck— clean (citizen-pwa + functions)npx vitest run— 53 files, 318 tests, all passRelated
Closes post-impl-review findings from
docs/no-need-to-schedule-graceful-quasar.mdSummary by CodeRabbit
New Features
Improvements